Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(http): set I/O timeout to 1 minute rather than whole request timeout #5916

Merged
merged 1 commit into from
Aug 25, 2024

Conversation

link2xt
Copy link
Collaborator

@link2xt link2xt commented Aug 25, 2024

Before the fix HTTP client
had no connection timeout,
so it only had a chance
to test one IPv6 and one IPv4
address if the first addresses timed out.
Now it can test at least 4 addresses
of each family and more if some addresses
refuse connection rather than time out.

// if request timeout is set to 5 minutes
// and connection timeout is set to 1 minute.
//
// We do not set write timeout because `reqwest`
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For IMAP and SMTP we set both read and write timeout in connect_tcp_inner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened an issue about the lack of API to set write timeout: seanmonstar/reqwest#2403

We may also switch to using hyper directly and establishing connections manually in the future as we already have generic code to handle IMAP and SMTP connection attempts and TLS session establishment, but for now this minimal change is good enough to fix the bug and allow Delta Chat to test multiple IP addresses from DNS response.

Before the fix HTTP client
had no connection timeout,
so it only had a chance
to test one IPv6 and one IPv4
address if the first addresses timed out.
Now it can test at least 4 addresses
of each family and more if some addresses
refuse connection rather than time out.
@link2xt link2xt force-pushed the link2xt/http-timeout-fix branch from 4bb4dc4 to 1109d5a Compare August 25, 2024 15:42
@link2xt link2xt merged commit f912bc7 into main Aug 25, 2024
37 checks passed
@link2xt link2xt deleted the link2xt/http-timeout-fix branch August 25, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants